deck: Assigned unique id to each card. #14

sethdivyansh · 2024-05-28T11:45:21Z

Created an ID in the format card-type-color-value.
Added a sameCardCount array to uniquely identify cards of the same type and color.
Generated a unique ID using the format card-type-color-value-sameCardCount[id].

Fixes #4

Reason for choosing custom unique id instead of UUID etc

Custom unique id contains meaningful information about the card that may help in future debuging. For example: card-Special-Red-skip-2 immediately conveys that the card is a red "skip" card and it is the second one of its kind.
Generating custom id is computationally cheaper and simpler compared to generating a UUID.
IDs like card-Special-Red-skip-2 are human-readable, making it easier for developers and testers to trace issues, verify game logic, and understand game states during testing and development. UUIDs, being long and non-descriptive, lack this readability.

Self-review checklist

Self reviewed the changes for clarity and maintainability (variable names, code reuse, readability, etc.).
Highlights technical choices and bugs encountered.
Calls out remaining decisions and concerns.
Automated tests verify logic where appropriate.
Each commit is a coherent idea.
Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

Visual appearance of the changes.
End-to-end functionality of buttons, interactions and flows.
Corner cases, error conditions, and easily imagined bugs.

kuv2707 · 2024-05-28T12:13:32Z

backend/uno-game-engine/deck.js

@@ -60,7 +60,16 @@ export default function getShuffledCardDeck() {
 */
 function makeCard(type, color, value) {
    //todo: Implement unique identification of cards by assigning an id to each card
-    return { type, color, value };
+    let card_num = [];


The card_num array will be lost when the function returns. We'd need to persist the array data across multiple function calls.

Yes, I realized my mistake later. I have corrected it; please review the updated version.

kuv2707 · 2024-05-30T09:34:30Z

@sethdivyansh Please rebase your branch against upstream/main and resolve the merge conflicts.
You'd also have to deal with types now that this module is converted to TS.

I'l merge it then.

sethdivyansh · 2024-05-30T11:19:43Z

@kuv2707 I have resolved all the conflicts. Please review the PR and merge it.

backend/uno-game-engine/types.d.ts

kuv2707 · 2024-05-30T17:09:56Z

backend/uno-game-engine/deck.ts

@@ -57,7 +58,13 @@ export default function getShuffledCardDeck(): Array<UNOCard> {
 */
 function makeCard(type: CardType, color: CardColor, value: CardValue): UNOCard {
    //todo: Implement unique identification of cards by assigning an id to each card
-    return { type, color, value, id: undefined };
+    let id = `card-${type}-${color}-${value}`;


Prefer const for variables which do not change. Eslint should have given an error.

kuv2707 · 2024-05-30T17:11:03Z

backend/uno-game-engine/deck.ts

+    if (!sameCardCount[id]) sameCardCount[id] = 0;
+    sameCardCount[id]++; // increment the count of same cards to assign unique id
+
+    let uid = `${id}-${sameCardCount[id]}`;


sethdivyansh · 2024-05-30T18:07:24Z

@kuv2707 I have made all the changes.

kuv2707 · 2024-05-31T06:14:37Z

@sethdivyansh Please add the "Fixes" clause in the commit message.

sethdivyansh · 2024-05-31T06:43:31Z

@kuv2707 Done

kuv2707 · 2024-05-31T12:14:29Z

@sethdivyansh Please add the motivation to use a custom-generated ID instead of approaches like uuid etc in the PR description.
There had been some discussion regarding it in the issue page.

This PR is ready to be merged. Will do it once the event begins officially.

sethdivyansh · 2024-05-31T12:51:34Z

@kuv2707 @shivansh-bhatnagar18 I have mentioned the reasons for choosing custom IDs. If you feel using UUIDs is a better approach, please let me know, and I will make the desired changes accordingly.

kuv2707 · 2024-05-31T12:54:30Z

No, this approach is fine. I just wanted you to mention the reasoning in the PR description for future reference.

kuv2707 · 2024-05-31T19:54:56Z

backend/uno-game-engine/deck.ts

@@ -57,7 +58,13 @@ export default function getShuffledCardDeck(): Array<UNOCard> {
 */
 function makeCard(type: CardType, color: CardColor, value: CardValue): UNOCard {
    //todo: Implement unique identification of cards by assigning an id to each card


@sethdivyansh Remove this todo comment.

I have removed the comment.

- Created an ID in the format `card-type-color-value`. - Added a `sameCardCount` array to uniquely identify cards of the same type and color. - Generated a unique ID using the format `card-type-color-value-sameCardCount[id]`. Fixes shivansh-bhatnagar18#4

kuv2707 · 2024-06-01T10:31:16Z

Merged, thanks @sethdivyansh !

sethdivyansh force-pushed the fix/issue-#4 branch from fd7e42b to 77ea2e9 Compare May 28, 2024 11:49

kuv2707 reviewed May 28, 2024

View reviewed changes

sethdivyansh force-pushed the fix/issue-#4 branch from 77ea2e9 to 9cfc7ae Compare May 28, 2024 16:36

Varun-Kolanu force-pushed the master branch 2 times, most recently from f828c52 to a008630 Compare May 28, 2024 16:56

kuv2707 mentioned this pull request May 30, 2024

Make it possible to uniquely identify cards #4

Closed

sethdivyansh force-pushed the fix/issue-#4 branch from 9cfc7ae to 91d4931 Compare May 30, 2024 11:10

kuv2707 reviewed May 30, 2024

View reviewed changes

backend/uno-game-engine/types.d.ts Outdated Show resolved Hide resolved

sethdivyansh force-pushed the fix/issue-#4 branch from 35ad142 to 7bf32c6 Compare May 30, 2024 11:58

kuv2707 requested changes May 30, 2024

View reviewed changes

sethdivyansh changed the title ~~deck: Implement unique identification of cards by assigning an id to each card~~ deck: Assigned unique id to each card. May 30, 2024

sethdivyansh force-pushed the fix/issue-#4 branch from 7bf32c6 to c851162 Compare May 30, 2024 18:03

sethdivyansh requested a review from kuv2707 May 30, 2024 18:05

kuv2707 added the approved: Kislay label May 31, 2024

kuv2707 removed the approved: Kislay label May 31, 2024

sethdivyansh force-pushed the fix/issue-#4 branch from c851162 to c1943b7 Compare May 31, 2024 06:37

sethdivyansh closed this May 31, 2024

sethdivyansh reopened this May 31, 2024

kuv2707 mentioned this pull request May 31, 2024

Feature UUID #24

Closed

kuv2707 added the approved: Kislay label May 31, 2024

kuv2707 reviewed May 31, 2024

View reviewed changes

sethdivyansh force-pushed the fix/issue-#4 branch from c1943b7 to c122a60 Compare May 31, 2024 20:11

sethdivyansh force-pushed the fix/issue-#4 branch from 638c172 to aad9592 Compare May 31, 2024 21:27

sethdivyansh force-pushed the fix/issue-#4 branch from aad9592 to 81d9652 Compare June 1, 2024 07:19

kuv2707 merged commit f05595c into shivansh-bhatnagar18:master Jun 1, 2024
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deck: Assigned unique id to each card. #14

deck: Assigned unique id to each card. #14

sethdivyansh commented May 28, 2024 •

edited

Loading

kuv2707 May 28, 2024

sethdivyansh May 28, 2024

kuv2707 commented May 30, 2024

sethdivyansh commented May 30, 2024

kuv2707 May 30, 2024

kuv2707 May 30, 2024

sethdivyansh commented May 30, 2024

kuv2707 commented May 31, 2024

sethdivyansh commented May 31, 2024 •

edited

Loading

kuv2707 commented May 31, 2024

sethdivyansh commented May 31, 2024

kuv2707 commented May 31, 2024

kuv2707 May 31, 2024

sethdivyansh Jun 1, 2024

kuv2707 commented Jun 1, 2024

deck: Assigned unique id to each card. #14

deck: Assigned unique id to each card. #14

Conversation

sethdivyansh commented May 28, 2024 • edited Loading

Reason for choosing custom unique id instead of UUID etc

kuv2707 May 28, 2024

Choose a reason for hiding this comment

sethdivyansh May 28, 2024

Choose a reason for hiding this comment

kuv2707 commented May 30, 2024

sethdivyansh commented May 30, 2024

kuv2707 May 30, 2024

Choose a reason for hiding this comment

kuv2707 May 30, 2024

Choose a reason for hiding this comment

sethdivyansh commented May 30, 2024

kuv2707 commented May 31, 2024

sethdivyansh commented May 31, 2024 • edited Loading

kuv2707 commented May 31, 2024

sethdivyansh commented May 31, 2024

kuv2707 commented May 31, 2024

kuv2707 May 31, 2024

Choose a reason for hiding this comment

sethdivyansh Jun 1, 2024

Choose a reason for hiding this comment

kuv2707 commented Jun 1, 2024

sethdivyansh commented May 28, 2024 •

edited

Loading

sethdivyansh commented May 31, 2024 •

edited

Loading